-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pb-rust: codegen via prost
+ prost-reflect
#207
pb-rust: codegen via prost
+ prost-reflect
#207
Conversation
CC @woodruffw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, this helps clean up uses of protobuf specs in e.g. sigstore/sigstore-rs#311 a lot 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @tnytown!
JIT generation is indeed more idiomatic, although it's slightly out-of-sync with what the rest of this repo does. I have no strong opinion here, though, so I'm happy to merge with this approach assuming @kommendorkapten and @haydentherapper have no objections.
(Planning note: once merged, I'll do another RC release to crates.io.) |
@tnytown You'll need to rebase here 🙂 |
62d183f
6376ca8
to
62d183f
Compare
@woodruffw rebased! |
Hmm, tests are failing. Taking a look ... |
5dbaa14
to
68d3ec1
Compare
Let's get this in following #213. That way we can do a |
68d3ec1
to
0326f8e
Compare
take N+1 Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
This reverts commit 8283471. Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
0326f8e
to
cdab22b
Compare
This should be ready for merge again, modulo the bump to 0.3 in |
protobuf-specs/gen/pb-rust/Cargo.toml Line 3 in 1ef5b8e
Edit: From the failing test, I think you just need to regenerate the generated code |
Signed-off-by: Andrew Pan <[email protected]>
Oops! I bumped the version and reran |
Summary
Generate Protobuf structs via
prost
, handling the Protobuf JSON mapping withprost-reflect
.This approach has a few benefits:
base64
encoding of thebytes
type automatically.i64
correctly.Unfortunately,
prost
doesn't handle required fields, so we're still stuck with unwrappingOption
manually.I also took the liberty of generating the structs JIT with
build.rs
rather than using a separatecodegen
step. This slows down CI for Rust significantly since we have to run everything through the Docker container, but this approach is more idiomatic and won't clutter the tree with generated files. We may want to look into caching the build container somehow.Release Note
Documentation